dart_skills_lint reduce enforced cognitive complexity - #212
Conversation
- Eliminate named record return types in _buildContext and _loadIgnores in favor of standard Future<T> single-value returns - Audit and document _parse* methods in config_parser.dart and helper methods in test suites - Extract top-level test helpers _createMockRelease and _runInstallScriptTest in install_script_test.dart to lower main function cognitive complexity score - Update definition-of-done skill fail-threshold to 20
There was a problem hiding this comment.
Code Review
This pull request refactors several files in the dart_skills_lint tool to reduce cognitive complexity and improve code readability, primarily by extracting helper methods in config_parser.dart, validation_session.dart, validator.dart, and test files. Additionally, the cognitive complexity threshold in the definition of done was lowered from 48 to 20. Feedback on the changes identifies an unused normalizedSkillPath parameter in the newly extracted _getIgnoresForSkill method, which should be removed from both the method definition and its call site.
| final SkillsIgnores ignores = await _getIgnoresForSkill( | ||
| localIgnoreFile, | ||
| normalizedSkillPath, | ||
| rootDir, | ||
| loadedIgnoresCache, | ||
| ); |
There was a problem hiding this comment.
Remove the unused normalizedSkillPath argument from the call to _getIgnoresForSkill.
| final SkillsIgnores ignores = await _getIgnoresForSkill( | |
| localIgnoreFile, | |
| normalizedSkillPath, | |
| rootDir, | |
| loadedIgnoresCache, | |
| ); | |
| final SkillsIgnores ignores = await _getIgnoresForSkill( | |
| localIgnoreFile, | |
| rootDir, | |
| loadedIgnoresCache, | |
| ); |
| Future<SkillsIgnores> _getIgnoresForSkill( | ||
| String? localIgnoreFile, | ||
| String normalizedSkillPath, | ||
| Directory rootDir, | ||
| Map<String, SkillsIgnores> loadedIgnoresCache, | ||
| ) async { |
There was a problem hiding this comment.
|
|
Addressed in fbb08af: Removed unused |
The goal of this pr was to make code easier to read an maintain. We dropped the cognitive complexity floor from 48 to 20.
Definition of done eval #212 (comment)
As a note: What a terrible agent pr description.
Model 3.5 pro preview
Relevant prompts
I reviewed the changes manually and ended up thinking that several of the methods that were added to reduce complexity needed dart docs.
Found the model had a preference for methods that have 2 return types that are kind of related. Like a text blob and if it needed to be modified. For each of the cases I asked it to try on just the at issue method.
Then I realize that the agent didnt enforce the new lower number.
Some back and forth about the right place to put a new test. decided a new file made sense.
Agent pr description below
Summary
Refactors internal helper return types away from named record tuples (
({ ... })) to standard single-value returns and reduces cognitive complexity across test suites to meet the threshold of 20.Motivation and Context
Named record return tuples on private helper methods (
_buildContextand_loadIgnores) added unnecessary syntactic overhead and tuple-unpacking noise at call sites. Additionally, nested helper closures insidemain()in test suites (test/install_script_test.dart) caused cognitive complexity scores to accumulate onmain()and exceed the repository's complexity limit. Moving those helpers to top-level private declarations correctly isolates their complexity scores.Related Issues
Related to #211
What changed
lib/src/validator.dart: Inlined frontmatter YAML parsing into_buildContextand refactored its return signature fromFuture<({SkillContext context, List<ValidationError>? fatalErrors})>toFuture<SkillContext?>, passingfatalErrorsas an out-parameter.lib/src/validation_session.dart: Extracted ignore path resolution into_resolveIgnorePathso that_loadIgnoresreturnsFuture<SkillsIgnores>directly instead of a record tuple. Added dartdoc clarifying "root" vs "entity" in_processRootSkillEntity. Removed a duplicate docstring block aboveprocessSkillRoot.lib/src/config_parser.dart: Audited and documented_parse*helper methods to clearly contrast global baselines vs path-specific overrides and single-item parsers vs collection iterators.test/install_script_test.dart: ExtractedcreateMockReleaseandrunInstallScriptTestout ofmain()to top-level private helpers_createMockReleaseand_runInstallScriptTest, droppingmain()'s cognitive complexity score from 48 to 20. Added dartdoc for_createMockRelease.test/rules_md_consistency_test.dart: Added dartdocs for_findSeverityMismatchesand_findFixableMismatches..agents/skills/definition-of-done/SKILL.md: Updated cognitive complexity fail-threshold in the verification checklist to 20.Testing Instructions
dart format .to verify formatting.dart analyze --fatal-infosto verify static analysis.dart run cognitive_complexity --fail-threshold 20 lib testto confirm all declarations score <= 20.dart testto verify all 210 unit tests pass.dart run bin/cli.dart -d .agents/skillsto validate all repository skills.